Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#133 code parser alternative #146

Merged
merged 135 commits into from
Oct 9, 2023
Merged

#133 code parser alternative #146

merged 135 commits into from
Oct 9, 2023

Conversation

m7pr
Copy link
Contributor

@m7pr m7pr commented Sep 29, 2023

Fixes #133

Alternative to #139

@m7pr m7pr added the core label Sep 29, 2023
@m7pr m7pr mentioned this pull request Sep 29, 2023
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like format_expression doesn't make sense anymore. We rather need style_string_code instead.

R/qenv-eval_code.R Show resolved Hide resolved
R/qenv-get_code.R Outdated Show resolved Hide resolved
R/qenv-constructor.R Show resolved Hide resolved
R/qenv-eval_code.R Outdated Show resolved Hide resolved
@m7pr
Copy link
Contributor Author

m7pr commented Oct 2, 2023

Hey @gogonzo this isn't working at all, I just pushed the code at the end of the day so it's secured by being send to a remote repository. It should be ready for testing today at the end of the day

@m7pr
Copy link
Contributor Author

m7pr commented Oct 2, 2023

R/utils-code_dependency.R Outdated Show resolved Hide resolved
R/qenv-class.R Outdated Show resolved Hide resolved
R/qenv-get_code.R Outdated Show resolved Hide resolved
R/qenv-get_code.R Outdated Show resolved Hide resolved
R/qenv-get_code.R Outdated Show resolved Hide resolved
R/qenv-get_code.R Outdated Show resolved Hide resolved
R/qenv-get_code.R Outdated Show resolved Hide resolved
R/qenv-get_code.R Outdated Show resolved Hide resolved
R/qenv-get_code.R Outdated Show resolved Hide resolved
m7pr and others added 2 commits October 3, 2023 11:06
Co-authored-by: Aleksander Chlebowski <[email protected]>
Signed-off-by: Marcin <[email protected]>
Copy link
Contributor

@chlebowa chlebowa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I move that

  1. Only the generic constructor is mentioned in usage.
    image
  2. new_qenv, eval_code be documented in one file. Perhaps get_code as well.

Also, since we do require the user to tag code lines, I wonder whether we should extend it to have them tag all lines and not try to find dependencies automatically.

Then we could simply

cc <- c("sp <- 'virginica' #@effect ii", "i <- iris #@effect ii", "ii <- subset(i, Species == sp) #@effect ii")
ex <- parse(text = cc)
lines <- attr(ex, "srcfile")$lines
# pseudo
l <- grep("@effect ii", lines, value = TRUE)

Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails

  q <- new_qenv()
  q <- eval_code(q, expression({
    a <- 1
    b <- 2
  }))  

  testthat::expect_identical(
    get_code(q, names = "a"),
    "a <- 1"
  )

I think curly brackets should be removed either before inserting code to @code. Thanks to this trace in qenv.error will have no brackets also.

R/qenv-class.R Outdated Show resolved Hide resolved
R/qenv-get_code.R Outdated Show resolved Hide resolved
R/qenv-get_code.R Outdated Show resolved Hide resolved
R/qenv-get_code.R Outdated Show resolved Hide resolved
R/qenv-get_code.R Outdated Show resolved Hide resolved
tests/testthat/test-code_dependency.R Outdated Show resolved Hide resolved
tests/testthat/test-code_dependency.R Outdated Show resolved Hide resolved
tests/testthat/test-code_dependency.R Outdated Show resolved Hide resolved
tests/testthat/test-code_dependency.R Outdated Show resolved Hide resolved
tests/testthat/test-code_dependency.R Outdated Show resolved Hide resolved
m7pr and others added 4 commits October 3, 2023 14:41
Co-authored-by: Dawid Kałędkowski <[email protected]>
Signed-off-by: Marcin <[email protected]>
Co-authored-by: Dawid Kałędkowski <[email protected]>
Signed-off-by: Marcin <[email protected]>
Co-authored-by: Dawid Kałędkowski <[email protected]>
Signed-off-by: Marcin <[email protected]>
Co-authored-by: Dawid Kałędkowski <[email protected]>
Signed-off-by: Marcin <[email protected]>
@chlebowa
Copy link
Contributor

chlebowa commented Oct 6, 2023

It seems a list containing a symbol cannot be unlisted 🤔

@m7pr
Copy link
Contributor Author

m7pr commented Oct 6, 2023

Will have a look @chlebowa . You are good to go!

@chlebowa
Copy link
Contributor

chlebowa commented Oct 6, 2023

I adapted lang2calls slightly and now everything works consistently.

m7pr added 2 commits October 9, 2023 12:47
- it is just a wrapper that works on a text or expression with srcref attributes
- it is not included in get_code
- it is separated from eval_code
@m7pr
Copy link
Contributor Author

m7pr commented Oct 9, 2023

Hey, we had a debate with @gogonzo on the final form of the current stage of code parser.
We decided to leave get_dependency_code the way it is as an unexported function.
We only changed the format of eval_code so it returns characters so in the future we can apply get_dependency_code on it. There are potential ideas on the simplification of get_dependency_code implementation, but those are minor and can be addressed in the future.

@gogonzo would you mind taking a final look at the commit with recent changes that we discussed 193a52e
?

Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code_dependency covers cases which we desperately needed in teal. I accept this solution which can be now implemented on the refactor branch, where we can detect edge cases. Please merge to main once GH checks pass

@m7pr
Copy link
Contributor Author

m7pr commented Oct 9, 2023

Amazing, created a card for potential minor improvements that we can provide in the future #154

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023

badge

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  ------------------------
R/include_css_js.R               7       7  0.00%    12-20
R/qenv-concat.R                 10       0  100.00%
R/qenv-constructor.R            12       0  100.00%
R/qenv-eval_code.R              49       2  95.92%   95, 104
R/qenv-get_code.R               18       0  100.00%
R/qenv-get_var.R                19       0  100.00%
R/qenv-get_warnings.R           24       0  100.00%
R/qenv-join.R                   46       0  100.00%
R/qenv-show.R                    1       1  0.00%    16
R/qenv-within.R                  7       0  100.00%
R/utils-code_dependency.R      190       7  96.32%   37, 42, 252-253, 317-320
R/utils.R                       12       0  100.00%
TOTAL                          395      17  95.70%

Diff against main

Filename                     Stmts    Miss  Cover
-------------------------  -------  ------  --------
R/qenv-eval_code.R              +1       0  +0.09%
R/utils-code_dependency.R     +190      +7  +96.32%
R/utils.R                       -7       0  +100.00%
TOTAL                         +184      +7  +0.44%

Results for commit: 864cc52

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Code Parser]: Ability to figure out object dependencies based on a static code
3 participants